-
Notifications
You must be signed in to change notification settings - Fork 8k
Update minimum required PHP version in Autotools build system #20933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
What was the issue with 7.4 for gen_stub? I had intentionally avoided using things that would require 8.0+ in my cleanups |
In PHP 8.4: and in PHP-8.5 and master: Fatal error: Uncaught TypeError: Typed property EvaluatedValue::$value must be an instance of mixed, string used in /opt/php-src/build/gen_stub.php:2357 |
|
@DanielEScherzer Yes, basically Nikita's request was not to increase the minimum required PHP version to a too recent version, but he told this a few ago already when PHP 8.0 was only a couple of years old. Now I think it's OK to make this change, especially because proper support for PHP 7.4 is broken for a while ^^ So at least the fact that no one complained confirms that we won't break too many workflows :) And then the next question: is it ok to increase the minimum version to PHP 8.1 so that readonly properties can be used? IMO it's also a viable option besides PHP 8.0. |
I think so, yes. Probably this version might be limited in cases when building PHP from source on systems that have some old PHP installed. And I don't see any reason why this would cause any issues out there. Edit: Another possible reason for very very old PHP version used here was that once this version was hardcoded in build/php.m4 file directly (in the |
At the time of writing, PHP found on host system must be at least 8.0 for build/gen_stub.php and Zend/zend_vm_gen.php scripts to work. Minimum required version is updated to 8.1 to still support building PHP from source on any possible old systems, and enabling new features in build/gen_stub.php, such as readonly properties.
a5838c0 to
69ec11f
Compare
|
LGTM on my part, but I don't know if this needs more discussion (at least among main contributors) 🤷 |
|
Please don't raise requirements for gen stub. In fact it should be fixed to actually work with all versions since 7.4. The reason is that it is an essential tool for building php itself, and 7.4 is the last version that does not require php to build itself. So you can build 7.4 to then build 8.x. Otherwise it forces to use binary 3rd party or distro packages, increasing the supply chain attack surface. |
Which version would be then suitable to update this requirement? PHP 9? PHP 10?
PHP isn't required to build PHP itself. This is only for development of php-src, when some stub.php file changes to regenerate its arginfo.h file. |
I would raise the requirement when needed. Do you need to raise the requirement? Like for enums or some kind of very important feature? |
So you can then use 7.4 to build 8.0 and use 8.0 to build 8.1 and so on. |
|
I just checked the code (and submitted a tiny fix for 7.4). I really don't see the need to raise the requirements. I think the right time to do so is when PHP Parser is updated to a version with higher requirements, which is probably going to be 6.0 I suppose. For one more reason, I run patched versions of PHP and I need to regenerate the stubs, otherwise I have to maintain the patch with both the stub file and the generated file diff. |
@TimWolla This comment is rather useless. That's obvious, but why? Just so you can use some new fancy syntax in the gen_stub? |
To reduce the maintenance effort by the PHP core team who shouldn't need to remember what's valid in a PHP version that was released 6 years ago and is EOL for 3 years. |
But this is a tautology! It's always a good thing to reduce the core team burden, but then with the same reasoning you should drop support for every library, compiler, build tools like autoconf, re2c, bison, etc, that is not the last stable release... Clearly it's always a trade-off between having the best support for all kinds of environments and the cost in effort for doing so. For a tool written in PHP and builds PHP itself, I think it's unnecessary to use union types, enums, constructor parameters promotion and fancy stuff like that. So please keep support for 7.4 until it's necessary to use a PHP-Parser that requires a higher version. At that point the upgrade is forced. |
Thank you! However, there is also Zend/zend_vm_gen.php that needs to be fixed then in PHP-8.5 branch and ensuring it works for PHP 7.4: Well, I won't push moving this forward if there is so determined position about 7.4. However, we all can agree that PHP 7.4 wasn't used to generate arginfo.h files for at least since PHP 8.4. Neither for packages out there, even if there are patched stub.php files. Also, I haven't noticed anyone reporting any bug regarding this. Sooner or later bumping this version will be needed but I don't want to convince you since you already have such strong belief this is so important. I don't generally contribute to these PHP scripts but if I would, I'd want it bumped for having better code features. |
I agree that a clean bump for master is desirable (i.e. first increase the official minimum as done in this PR and then make use of the features). Given some of the newer features already slipped on and that additional type annotations are already made by means of a comment, it indicates that there is an appetite from core developers to be able to use more recent PHP features (particularly around the improved type safety). |
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
|
Just for the side info, how this is resolved in CMake at this point: Because, there is another PHP script in master branch that requires PHP 8.0+: ext/phar/makestub.php (if executed by PHP 7.4 it has some issues with missing |
If you're really doing this, I wouldn't recommend such approach actually. Or at least you need to be aware of some issues along the way. For example, I also have one patched stub.php and arginfo.h file and I found it is easier to patch the arginfo.h file along with stub.php). PHP with Autotools doesn't automatically run the |
|
@TimWolla I think this question would be a good candidate to be included in a fitting policy RFC if you or someone proposed one next time :) |
At the time of writing, PHP found on host system must be at least 8.0 for build/gen_stub.php and Zend/zend_vm_gen.php scripts to work.